Draft PR for discussion: transition to CSS varaibles only for type#1044
Draft PR for discussion: transition to CSS varaibles only for type#1044stephaniehobson wants to merge 1 commit intomainfrom
Conversation
| --title-xs-line-height: 1.166; | ||
| --title-2xs-size: 1.25rem; | ||
| --title-2xs-line-height: 1.2; | ||
| --title-2xl-size: 3.5rem; |
There was a problem hiding this comment.
I've been toying with the idea of variable names that make it clear that they morph with media queries. Alternative name ideas:
--scale-title-2xl-size: 3.5rem;(colours could usetheme, nice because it gives a clue of the variable value)--let-title-2xl-size: 3.5rem;(javascript reference: const vs let, mostly nice because its short, we should not have any const CSS vars, they should be Sass vars)--find-title-2xl-size: 3.5rem;(nice because all dynamic variables can use the same prefix)
There was a problem hiding this comment.
But if we follow the rule that CSS variables should ONLY be used for dynamic variables then maybe the prefix is unnecessary? But then, is there value in making it explicit for contributors and users of the system who don't know the rules?
There was a problem hiding this comment.
Minor preferences:
- scale/theme prefixes on CSS custom properties look good to me, I like the extra indication of change based on context
var()syntax doesn't bother me but happy to save a few characters with alias to Sass variables
There was a problem hiding this comment.
if we follow the rule that CSS variables should ONLY be used for dynamic variables then maybe the prefix is unnecessary
I'd agree here that prefixes are unnecessary if we only use CSS variables for dynamic values and don't use a sass alias (the var() reads really nicely)
There was a problem hiding this comment.
I guess the issue is it doesn't distinguish between values that are variable based on theme only (i.e. font family will change between themes but be static within that theme) vs. multiple factors (i.e. title size will change based on theme and across screen sizes)
stephaniehobson
force-pushed
the
css-vars
branch
from
f3f82aa to
1d59bc4
Compare
|
For color naming, I think it's great to have variables that can react to both theme (fx/moz) and color-scheme (light/dark). We'll need to update m24 naming to match Protocol's usage-based names (like 
Does more reliance on CSS properties and inheritance allow us to remove some of the link color mixins? |
|
(Might be out of scope for discussion) Could we replace the hover colors with a function? (Sass darken or CSS color-mix or relative colors) Two fewer tokens to maintain, easier to scale a consistent hover style if we add other link colors later |
janbrasna
left a comment
janbrasna
left a comment
There was a problem hiding this comment.
The failed check is just a linter being loud about whitespace, nothing more.
Couple of naive questions follow, esp. if the change to the px-to-rem mixin is intentional:
| --body-md-size: 0.875rem; | ||
| --body-sm-size: 0.75rem; | ||
| --body-xs-size: 0.688rem; | ||
| // --body-line-height: 1.5; doesn't change, no need to update |
There was a problem hiding this comment.
IIUC it's actually 1.2:
| // --body-line-height: 1.5; doesn't change, no need to update | |
| // --body-line-height: 1.2; doesn't change, no need to update |
not that it matters, just in case it's ever uncommented to not break the scale…
| --body-sm-size: 0.875rem; | ||
| --body-xs-size: 0.75rem; | ||
| --body-line-height: 1.2; | ||
| // --body-line-height: 1.2; doesn't change, no need to update |
There was a problem hiding this comment.
After the updates to L50–L58 also the other line-height title vars here above don't change (from 1.1 to 1.1), not sure how much of it you plan to omit.
This also applies to _firefox.scss theme where it is 1.5 before and also after mq adjustments.
There was a problem hiding this comment.
I've omitted all the unnecessary re-definitions in both Mozilla and Firefox theme files instead of commenting them out. The code is cleaner, hopefully it doesn't confuse anyone.
There was a problem hiding this comment.
Nice catch, thanks ;)
stephaniehobson
changed the title
|
Do Not Merge until Icon release is published |
stephaniehobson
force-pushed
the
css-vars
branch
2 times, most recently
from
2c31e57 to
a07a807
Compare
stephaniehobson
force-pushed
the
css-vars
branch
from
a07a807 to
dc5c79b
Compare
3 participants
Draft PR for discussion.